fix(acp-server): surface provider errors on session/prompt - #3161
fix(acp-server): surface provider errors on session/prompt#3161jangjoe wants to merge 2 commits into
Conversation
`session/prompt` used to silently resolve with `{stopReason: 'end_turn'}`
when the model provider returned a transport / status failure
(rate limit, overload, 5xx, connection loss, etc.). The engine already
classifies these as born-coded `Error2` instances with codes like
`provider.api_error` / `provider.overloaded` /
`provider.connection_error` / `context.overflow` and surfaces them
on `turn.ended{ reason: 'failed' }`, but the ACP layer only knew how to
map auth errors — every other failure was swallowed into `end_turn`
and clients had no way to tell the model service was unavailable.
Add `isProviderError()` in events-map.ts (sibling to `isAuthError()`)
and a third branch in both `AcpSession.onTurnEnded` and
`mapPromptLaunchError`: provider / context failures now reject with
`RequestError.internalError({ code, message }, "model provider
reported an error")`. The engine's full text rides in the JSON-RPC
`data` payload so clients can log it; the wire message stays generic
to keep provider-supplied PII out of every error toast. Auth failures
keep their existing `auth_required` mapping; content-filter failures
keep the legacy `refusal` mapping in `turnEndReasonToStopReason`.
The scripted provider helper gains `mockNextProviderError(code, message)`
so end-to-end tests can throw a coded provider error without going
through the network layer.
Resolve MoonshotAI#3107
🦋 Changeset detectedLatest commit: f06428b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Signed-off-by: Jang Joe <jangjoe@qq.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f06428b836
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| mockNextProviderError: (code, message) => { | ||
| queue.push({ throwError: new Error2(code, message) }); |
There was a problem hiding this comment.
When mockNextProviderError is called with anything other than provider.api_error, the queued Error2 is caught by the helper's existing requester.generate catch and reduced to an llm.failed.remote event with kind: 'unknown'; errorFromLlmMessage then reconstructs it as a default provider.api_error. Consequently this helper cannot exercise the newly added branches for context.overflow, provider.overloaded, auth failures, or the other accepted ErrorCode values, while the current provider.api_error test passes accidentally. Propagate a failure event kind that preserves the requested code or bypass that lossy catch.
Useful? React with 👍 / 👎.
1 participant
Resolve #3107
session/promptused to silently resolve with{stopReason: 'end_turn'}when the model provider returned a transport / status failure
(rate limit, overload, 5xx, connection loss, etc.). The engine already
classifies these as born-coded
Error2instances with codes likeprovider.api_error/provider.overloaded/provider.connection_error/context.overflowand surfaces themon
turn.ended{ reason: 'failed' }, but the ACP layer only knew howto map auth errors — every other failure was swallowed into
end_turnand clients had no way to tell the model service was unavailable.
Change
Add
isProviderError()inpackages/acp-server/src/events-map.ts(sibling to
isAuthError()) and a third branch in bothAcpSession.onTurnEndedandmapPromptLaunchError: provider /context failures now reject with
RequestError.internalError({ code, message }, "model provider reported an error"). The engine's full text rides in the JSON-RPCdatapayload so clients can log it; the wire message stays genericto keep provider-supplied PII out of every error toast.
auth_requiredmapping.provider.filtered) keep the legacyrefusalmapping inturnEndReasonToStopReasonand fall throughto the fixed generic message at launch (where no turn exists yet to
attach a refusal to).
The scripted provider helper gains
mockNextProviderError(code, message)so end-to-end tests can throw a coded provider error without going
through the network layer.
Test plan
packages/acp-server/test/events-map.test.ts— new unit tests forisAuthError/isProviderError/turnEndReasonToStopReason,covering every code in each set plus the cross-set negatives
(
provider.filteredmust not matchisProviderError).packages/acp-server/test/e2e-turn.test.ts—mapPromptLaunchErrorunit tests for
provider.api_error/context.overflow/provider.filtered(legacy) and a scripted end-to-end test thatthrows a coded provider error mid-turn and asserts the prompt
rejects as
internalErrorcarrying the engine message indata.